- 
                Notifications
    
You must be signed in to change notification settings  - Fork 246
 
[Windows] Support CPU shared memory (Client/Frontend) #551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
        
          
                src/python/library/tritonclient/utils/shared_memory/shared_memory_handle.h
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | extern "C" { | ||
| #endif | ||
| 
               | 
          ||
| #ifdef _WIN32 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a platform specific sub-struct with either HANDLE or int would clean up this logic as well.
Platform specific logic will be encapsulated within the sub-struct definition.
However, we must take care that we maintain client side backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is handled opaquely now through use of a void pointer, as discussed, and is used to initialize the appropriate type when creating the substruct. None of the client APIs needed modifications in our L0_shared_memory test, which gives me confidence that we did not break backwards compatibility with this change. Please let me know if you have other concerns.
| if "linux" in PLATFORM_FLAG: | ||
| platform_package_data += ["libcshm.so"] | ||
| elif PLATFORM_FLAG == "win_amd64": | ||
| platform_package_data += ["cshm.dll"] | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add an else block catching unsupported platform flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The platform flag is currently passed in from build_wheel.py, so, from our perspective, the possible platforms are all handled. If the platform does not contain the substring "linux" or is not "win_amd64", it will be "any". In this case, we do not include either shared library object.
Are you suggesting raising an error in the event the setup.py script is called independently? Like so:
if "linux" in PLATFORM_FLAG:
    platform_package_data += ["libcshm.so"]
elif PLATFORM_FLAG == "win_amd64":
    platform_package_data += ["cshm.dll"]
elif PLATFORM_FLAG != "any":
    raise Exception(f"Unsupported platform flag \"{PLATFORM_FLAG}\" specified.") fcee735    to
    479dccc      
    Compare
  
    479dccc    to
    43700e2      
    Compare
  
    43700e2    to
    f854d4a      
    Compare
  
    
Goal: Support CPU shared memory between the server and client for Windows
Server changes: triton-inference-server/server#7048